feat(experimentation): one warehouse connection per environment#7582
feat(experimentation): one warehouse connection per environment#7582Zaimwa9 wants to merge 1 commit into
Conversation
Zaimwa9
requested review from
gagantrivedi and
khvn26
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the WarehouseConnection model to restrict each environment to a single active warehouse connection, regardless of its type. This change includes a new migration, updated model constraints, and modified logic in the serializers and views to enforce this rule. Feedback highlights a potential migration failure if existing environments have multiple active connections of different types, necessitating a data migration. Additionally, the resurrection logic in the serializer is flagged as risky for data consistency, as it allows changing the warehouse_type of an existing record; it is suggested to only resurrect records that match the requested type.
| migrations.AddConstraint( | ||
| model_name="warehouseconnection", | ||
| constraint=models.UniqueConstraint( | ||
| condition=models.Q(("deleted_at__isnull", True)), | ||
| fields=("environment",), | ||
| name="unique_active_warehouse_per_env", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
This migration will fail if any environment currently has multiple active warehouse connections of different types. Since the previous constraint (unique_active_warehouse_per_type_and_env) allowed one connection per type, an environment could have both a Snowflake and a Flagsmith connection active. You should include a data migration to resolve these duplicates (e.g., by soft-deleting all but the most recent one) before applying this new constraint.
| environment=environment, | ||
| warehouse_type=warehouse_type, | ||
| deleted_at__isnull=False, | ||
| ) | ||
| .order_by("-deleted_at") | ||
| .first() | ||
| ) | ||
| if existing: | ||
| existing.deleted_at = None | ||
| existing.warehouse_type = warehouse_type |
There was a problem hiding this comment.
Resurrecting a soft-deleted connection and changing its warehouse_type is risky. If the connection's ID is referenced by other models (e.g., experiments), those references will now point to a connection of a different type, leading to data inconsistency. Since the new unique constraint only applies to active records (deleted_at__isnull=True), it is safer to only resurrect records that match the requested warehouse_type and create a new record otherwise.
| environment=environment, | |
| warehouse_type=warehouse_type, | |
| deleted_at__isnull=False, | |
| ) | |
| .order_by("-deleted_at") | |
| .first() | |
| ) | |
| if existing: | |
| existing.deleted_at = None | |
| existing.warehouse_type = warehouse_type | |
| environment=environment, | |
| warehouse_type=warehouse_type, | |
| deleted_at__isnull=False, | |
| ) | |
| .order_by("-deleted_at") | |
| .first() | |
| ) | |
| if existing: | |
| existing.deleted_at = None |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7582 +/- ##
=======================================
Coverage 98.50% 98.50%
=======================================
Files 1430 1431 +1
Lines 54245 54263 +18
=======================================
+ Hits 53435 53453 +18
Misses 810 810 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Enforce a single active warehouse connection per environment, regardless of warehouse type:
unique_active_warehouse_per_type_and_envconstraint (scoped towarehouse_type + environment) withunique_active_warehouse_per_env(scoped toenvironmentonly)warehouse_type) rather than matching by type0003_unique_connection_per_environmentremoves the old constraint and adds the new onetest_post__different_type_already_exists__returns_409to verify cross-type uniquenessHow did you test this code?
Updated tests.